[fix] Converge the desktop session when a gate is answered elsewhere (11/12) - #5690
[fix] Converge the desktop session when a gate is answered elsewhere (11/12)#5690ardaerzin wants to merge 12 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
6101ac7 to
f957a88
Compare
28314ae to
cc14c6a
Compare
f957a88 to
f34d0e1
Compare
cc14c6a to
0aa2522
Compare
f34d0e1 to
4223629
Compare
0aa2522 to
cfb9c84
Compare
4223629 to
fed6233
Compare
cfb9c84 to
defca8b
Compare
fed6233 to
0e3c2cb
Compare
defca8b to
de37daa
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
0e3c2cb to
86c74ff
Compare
de37daa to
94e03ca
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
86c74ff to
ea4d80e
Compare
94e03ca to
52d6e1f
Compare
ea4d80e to
e13f3f2
Compare
4233737 to
0c1808e
Compare
6c75925 to
266ee69
Compare
0c1808e to
3b92ff2
Compare
1daa19a to
9f23d05
Compare
3b92ff2 to
88c741d
Compare
9f23d05 to
2ef2028
Compare
88c741d to
5997eb1
Compare
4261b81 to
0da367b
Compare
A turn approved elsewhere (e.g. from mobile) replayed as still parked: the durable record log carries the paused turn's interaction_request but no interaction_response, so the folded resume kept its tool part in approval-requested. The desktop adoption guard reads that as 'the server is parked too' and refused to adopt, leaving a reloaded session stuck on the pre-approval transcript with the approval dock up. Records that continue past a paused done mean the gate was answered (a deny settles its own part via tool_result denied), so settle whatever is left awaiting.
…h relay Subscribes the ACTIVE conversation to GET /sessions/streams/watch (the M3 SSE relay mobile already consumes) and, on records-changed, revalidates the durable records and re-applies the same guarded adoption the revalidate-on-open pass uses — so a turn that advances elsewhere lands in an open tab within seconds instead of only on reload. One foreground-only EventSource per panel, throttled, skipped while this tab streams; a fatal close retries every 60s and the reload path stays the fallback.
…solved A resume whose harness never re-raises the gate (cold replay, or a client-built resume that lands as a fresh turn) consumed the human's decision without transitioning the durable row, leaving it pending and forever actionable on every surface. The runner now settles those rows from the turn's in-band answers on every exit path, with the verdict the human gave, and the records worker cancels any gate whose turn reached a terminal record without pausing — guarded so a live park is never swept.
…biguity `alive` outlives its turn and a turn parked awaiting approval also clears `running`, so "alive held by another turn + no running" cannot tell a lapsed previous turn (a legitimate handover) from a live-but-parked one. Resolving it as a handover — which it must be, or every follow-up turn on a warm session aborts — let a zombie beat from an older turn take the nest of a parked session, after which the user's approval resume reported is_current_turn=false and aborted. Record what IS knowable at the moment it happens: every displacement (heartbeat handover, cancel, steer, kill, orphan sweep) now tombstones the turn it displaced, and a tombstoned turn's beats are refused before touching any lock or the stream row. A zombie is by definition a turn that already lost the nest, so `displaced => dead` is the discriminator the locks cannot provide. The ambiguous state keeps resolving as a handover; only a never-displaced turn can reach it. Also stops a superseded turn's is_running=false beat from clearing the LIVE turn's `running`, and a cancelled turn's beat from re-acquiring `alive`. The heartbeat wire is unchanged: `is_current_turn: false` already means "abort", so the runner needs no change and no restart.
Mobile showed assistant text through `whitespace-pre-wrap`, so a reply containing markdown displayed literal fences, `#` headings and `-` bullets. Desktop renders the same content properly. Adopt Streamdown (the renderer the mobile design and AI Elements already name) for assistant TEXT parts. User messages stay literal — markdown in your own words inside a bubble is surprising. Reasoning stays plain: it is collapsed by default and streams as fragments that incomplete-markdown repair would make flicker. - `isLiveTextItem` restricts incomplete-markdown repair to the one text item the stream is still appending to; settled text must not get it or a message legitimately ending in `**` is silently rewritten. - Streamdown's defaults are re-scaled to the app's 12px type ramp via descendant selectors, and its `sidebar` role (absent from the token bridge) is re-surfaced onto `muted`. Semantic tokens only. - Code blocks keep their own `overflow-x-auto`; the prose root wraps long unbroken tokens so the page body never scrolls horizontally. - Raw HTML is neutered by Streamdown's default rehype-raw -> rehype-sanitize -> rehype-harden pipeline; links get an explicit `rel="noopener noreferrer"`.
The SSE relay never told the client how long to wait before reconnecting, so the interval was implementation-defined — an API restart or deploy dropped every open stream and they all came back at once. The generator now leads with a `retry:` preamble (AGENTA_SESSIONS_WATCH_RETRY_ MILLISECONDS, default 5s), emitted after SUBSCRIBE so no event can land in an unsubscribed window between the client's `open` and the first frame.
At every access-token refresh boundary the whole client 401s for one round trip. The Fern/axios layers refresh and retry transparently; an EventSource has no interceptor, so the relay died and then sat blind for a fixed 60s — and a persistent failure retried on a flat cadence with no jitter. Both hooks now reopen on a jittered exponential backoff (1s → 30s, reset on `open`) and attempt a session refresh first, so an expired token costs seconds instead of a minute. Mobile also throttles the reconnect revalidation to the 3s desktop already used, so a reconnect loop can no longer fan out into one full records refetch per attempt (real `records-changed` events stay instant).
…he nest
The supersession tombstone makes "`alive` held by another turn + no `running`" safe to read as
a handover. Three paths around it still act on state they read a moment earlier, and each one
lets a dead turn touch something it no longer owns.
A tombstoned turn re-pinned the session to its own replica. `claim_owner` ran before the
tombstone was checked, so every trailing beat from a killed turn renewed OWNER_TTL. Since
`claim_owner` never steals, that is precisely the key another replica has to wait out before it
can take the session over. The refusal now reads affinity instead of claiming it, which also
makes the comment above it ("refuse the beat before it touches ANY lock") true.
The handover deleted `alive` unconditionally after reading its owner. A real `_start_turn` can
land in that gap, and the beat would then delete the incoming turn's lock and tombstone it,
killing a turn that had just legitimately taken the session. It now compare-and-deletes on the
owner it read, via the existing `release_alive`. Losing the race leaves `acquired` False, which
is the truth.
Cancel, steer and kill cleared the locks and then tombstoned what they displaced. A beat from
the turn being displaced, arriving between the two, found `alive` free and nx-acquired it
straight back, so a cancelled session read as alive until the TTL ran out. All three now share
`_displace_turns`, which writes the tombstone first (and still re-reads after the clear, so a
turn that took the keys inside the window is tombstoned too).
`test_heartbeat_lock_races.py` pins all three by driving the real interleavings. Each test fails
on the code as it stood.
The records worker cancelled every pending gate in the session, and kept that safe by first asking the turns ledger whether the finished turn was still the latest one. That check is a TOCTOU guard: a turn that starts and parks between the ledger read and the cancel has its live gate swept along with the orphan, and the human's approval disappears from every surface. The guard was only ever needed because the cancel was unscoped. It now addresses the finished turn's own gates and nothing else. A newer turn carries a different `turn_id`, so its park is out of range by construction - the worker may lag arbitrarily far behind the stream without the question of ordering arising at all. With that, the ledger read and the `turns_service` dependency both go away. Nothing else is lost. Prior turns' unanswered gates are swept at turn start by the runner's `/sessions/interactions/cancel-stale` call, which is what that endpoint exists for; this sweep is the net for the one case that endpoint cannot see, a turn that finishes without ever pausing. `turn_id` is nullable on the row but the only producer (`createInteraction` in the runner) always sends it, so no reachable gate falls outside the scope. `only_turn_id` mirrors the existing `except_turn_id` filter one layer down.
…rd goes out A resume the harness never re-gates leaves its durable row `pending`, and the end-of-turn settle is what transitions it with the verdict the human actually gave. It ran from the `finally`, fire-and-forget, after the turn had already finished. That is after the terminal `done` record is public. `record()` hands events straight to the sink, so `done` is out the moment `finish()` runs, and the API's gate reconciliation cancels the still-pending row as soon as it consumes it. The settle then transitions a row that is already terminal, gets a 404, and an approval or denial the human gave is filed as an abandonment. The settle is now awaited, and awaited before `finish()` on both the completion and the error path. It emits nothing into the event stream (it deliberately does not go through `resolveInteractionToken`), so moving it earlier does not put a record after the terminal one. The `finally` call stays as the backstop for exits that reach neither branch (cancel, abort); the resolved-token set makes it a no-op when the ordered call already ran. The new test in `session-keepalive-approval.test.ts` journals both sides through the real `runTurn` and asserts `resolve` precedes `done`. It reports them in the other order without the fix.
The 3s spacing between revalidations was leading-edge only: every notification inside the window was discarded outright. On a resumed turn the batches arrive close together and the LAST one is the `done` batch that settles the approval, so the transcript could adopt a partial replay and then never hear about the rest. A parked session has no `runningElsewhere` poll to correct it, so it stayed stale until the user reloaded. A notification inside the window now schedules a single trailing revalidation for when the window closes. One trailing call covers any number of notifications: the callback refetches the whole record log, it does not apply a delta. The timer is cleared on teardown with the retry one.
Both watch clients revalidated from `onopen`, which fires as soon as the response headers arrive. Starlette flushes those BEFORE it starts iterating the stream generator, so the sequence could be: headers out, client refetches the record log, a change lands and publishes, and only then does the generator's `subscribe` complete. That change reaches neither the refetch nor the stream, and on an idle screen the fallback poll is zero, so the transcript stays stale until something else wakes it. The endpoint now emits a `ready` event once the subscription is live, right behind the `retry:` preamble, and both clients revalidate on that instead. `onopen` keeps only what it can honestly report: the connection is up, so reset the backoff. The new test drives the generator and asserts the channel is already subscribed by the time `ready` is yielded, which is the whole invariant.
5997eb1 to
3561f72
Compare
0da367b to
5e00f40
Compare
Context
Once a phone can answer approvals, the desktop tab looking at the same session has to notice. It did not: an approval answered elsewhere left the desktop showing "Approval needed to continue" until a manual reload, and sometimes after one.
Changes
Desktop settles a resumed turn's gate on replay and converges an open session through the watch relay from lane 9, refreshing the session before reopening a relay it finds dead.
Two backend correctness fixes sit underneath. Gates whose turn was consumed but never resolved are settled rather than orphaned as
pendingforever. And a displaced turn is treated as dead, which closes the parked-session lock ambiguity: previously a displaced turn's late heartbeat could re-arm a lock that a newer turn owned.Mobile also renders assistant messages as markdown here, so a reply reads the same on both surfaces.
Tests / notes
AgentChatSlicehooks and the transcript adapter. Small diff, high scrutiny: this is the shared agent chat.What to QA